Skip to content

Standardize structured logging and update runtime integration coverage - #161

Merged
arthur-zhang merged 7 commits into
mainfrom
codex/a
Jul 27, 2026
Merged

Standardize structured logging and update runtime integration coverage#161
arthur-zhang merged 7 commits into
mainfrom
codex/a

Conversation

@arthur-zhang

@arthur-zhang arthur-zhang commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Standardize production logging around structured slog APIs and explicit logger injection.
  • Update API, worker, session, webhook, vault, filestore, and workbench paths to preserve request and resource context safely.
  • Refresh runtime configuration and OTLP metrics design documentation.
  • Expand integration and unit test coverage across files, sessions, environments, skills, and webhooks.

Testing

  • Not run (not requested).

Summary by CodeRabbit

  • New Features

    • Added structured, contextual logging across API requests, background workers, migrations, and administrative operations.
    • Added webhook delivery and enqueueing components with endpoint-aware processing and retry handling.
    • Added centralized runtime dependency and logging configuration.
  • Security

    • HTTP logs now omit query values, credentials, authorization data, cookies, tokens, request bodies, and raw headers.
  • Bug Fixes

    • Improved error reporting and panic diagnostics while preserving existing API responses and operational behavior.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@arthur-zhang, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 30 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 870c07c5-55fc-4902-b3f8-166c21de7ee1

📥 Commits

Reviewing files that changed from the base of the PR and between 5b78111 and 5379fd8.

📒 Files selected for processing (48)
  • AGENTS.md
  • docs/design/be/runtime-configuration.md
  • internal/admin/handler.go
  • internal/agents/handler.go
  • internal/api/filestore_auth_test.go
  • internal/api/platform_mcp_vault_auth.go
  • internal/api/server.go
  • internal/api/server_test.go
  • internal/api/service_auth.go
  • internal/batches/handler.go
  • internal/batches/worker.go
  • internal/cleanup/worker.go
  • internal/codesessions/ingress.go
  • internal/codesessions/managed_agent_code_session.go
  • internal/codesessions/otlp_file_log.go
  • internal/codesessions/service.go
  • internal/codesessions/tool_permissions.go
  • internal/deployments/handler.go
  • internal/environments/handler.go
  • internal/environments/rclone_filestore_test.go
  • internal/environments/runner.go
  • internal/environments/runner_dependencies_test.go
  • internal/files/handler.go
  • internal/files/platform.go
  • internal/filestore/cleanup.go
  • internal/filestore/cleanup_test.go
  • internal/filestore/handler.go
  • internal/logging/context_usage_test.go
  • internal/mcpcatalogs/handler.go
  • internal/memory/handler.go
  • internal/messages/handler.go
  • internal/sessions/code_event_bridge.go
  • internal/sessions/event_effects.go
  • internal/sessions/event_payload.go
  • internal/sessions/service.go
  • internal/sessions/webhook_bridge.go
  • internal/skills/handler.go
  • internal/skills/seed.go
  • internal/vaults/handler.go
  • internal/workbench/console_platform_workbench.go
  • internal/workbench/console_platform_workbench_test.go
  • main.go
  • tests/environments_e2b_integration_test.go
  • tests/environments_runner_cloud_test.go
  • tests/files_api_test.go
  • tests/message_batches_api_test.go
  • tests/sessions_api_test.go
  • tests/skills_seed_test.go
📝 Walkthrough

Walkthrough

This PR standardizes Go runtime logging on injected log/slog loggers, refactors workers and webhooks around explicit dependencies, moves workbench dependencies onto a handler, and updates startup, API handlers, tests, and documentation accordingly.

Changes

Structured logging and startup

Layer / File(s) Summary
Logging contracts and command startup
.gitignore, AGENTS.md, internal/logging/*, cmd/*, main.go, internal/api/*, docs/design/be/*
Adds logger defaults and structured logging guidance, propagates loggers through server construction and commands, avoids query-sensitive HTTP log output, and returns startup errors for final handling in main.
API and domain handler logging
internal/{admin,agents,batches,codesessions,deployments,environments,files,filestore,memory,messages,sessions,skills,vaults}/*
Replaces log.Printf calls with handler- or service-owned structured logger calls while preserving HTTP responses and core control flow.
Worker and webhook APIs
internal/{cleanup,batches,filestore,skillprewarm,environments,webhooks}/*
Introduces instance-based workers, typed webhook enqueue input, injected webhook delivery, and structured worker error logging.
Workbench handler ownership
internal/workbench/*
Moves route handlers and persistence helpers onto workbenchHandler, which owns persistence, upstream configuration, and logging dependencies.
Integration call sites and tests
tests/*, internal/**/*_test.go
Updates constructor calls, log capture, webhook execution, and worker execution to the new APIs and validates structured logger behavior.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.94% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: structured logging standardization and broader runtime/test coverage updates.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/a

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@duckpr duckpr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DuckPR reviewer: opencode
Model: anthropic/glm-5.2

ℹ️ 没有关键问题——少量改进建议见行内评论。这是一次净正向的重构:安全维度(query/body/userinfo 脱敏)、正确性维度(logger 注入、无 nil 解引用、无丢失日志信号)均无新增问题。下面的建议集中在测试一致性上。

Reviewed changes — 本 PR 将后端生产日志统一迁移到结构化 log/slog,并显式注入 logger;同时扩充了 files/sessions/environments/skills/webhooks 的集成与单测覆盖。

  • 统一 slog 日志 API 并显式注入 logger — 把几乎所有 log.Printf/log.Fatalf/log.Println 迁移到 slog,handler/worker/service 通过构造函数注入 *slog.Logger,边界统一用 logging.LoggerOrDefault 兜底;config.Config 不承载 logger。
  • 新增 AGENTS.md "Go 日志规范" 一节 — 明确 slog-only、显式注入、*Context 变体、snake_case 属性名、错误放 error 字段、禁止记录敏感 payload、业务包不得 Fatal/Panic/os.Exit 等约束。
  • 重命名 internal/observabilityinternal/logging — 新增 LoggerOrDefault helper 及对应单测。
  • 安全脱敏改进(净正向)http_log.go 改为只记录 r.URL.Path(原 r.URL.RequestURI() 会泄漏 query/OAuth state);platform_mcp_vault_auth.go 引入 platformMCPLogHost() 只返回 parsed.Host(剥离 userinfo/path/query/fragment);codesessions/ingress.go 移除 worker OTLP/bad-request 日志中的 body/query/user_agent,改为只记录计数与白名单元数据。
  • workbench handler 重构为结构体方法 — 新增 workbench_handler.go,把包级函数转为 workbenchHandler 方法并持有 store/upstream/logger
  • 新增 logger 注入测试messagesTestHandlerUsesInjectedLogger)和 webhooksTestEnqueuerUsesOwnedLogger)用 JSONHandler + 结构化字段断言验证 component/msg 等属性。
  • 更新设计文档otlp-metrics-api.mdruntime-configuration.md 同步新的日志字段名。

⚠️ workbench handler 新增 6 处结构化日志,但测试未覆盖 logger 注入

本 PR 在 console_platform_workbench.gohandleWorkbenchGeneratePrompt 路径新增了 6 处 h.logger.*Context(...) 调用(fallback/start/stream,涉及 upstream_hoststatusreasonerror 等字段),但 console_platform_workbench_test.go 只是把所有测试机械迁移到 newWorkbenchHandler(..., nil),对日志输出零断言。

这与本 PR 在同一批次里为 messageswebhooks 新增的 logger 注入测试形成明显反差——后两者都用 JSONHandler + 结构化字段断言验证了 component/msg。workbench 是本次改动量最大的 handler,反而没有等价覆盖。upstream_host 字段虽然按构造只取 host,但没有任何测试验证它确实不含完整 URL 或凭据。

建议补一个与 messages.TestHandlerUsesInjectedLogger 同形的测试:注入 buffer-backed logger,触发 fallback 路径,断言 msg/component/reason 等字段,并可顺带验证 upstream_host 只含 host。

Technical details
# workbench handler logger 注入测试缺失

## Affected sites
- `internal/workbench/console_platform_workbench.go:1688``h.logger.InfoContext(...)` "workbench generate prompt upstream start",记录 `upstream_host`/`model`/`task_chars`/`thinking`
- `internal/workbench/console_platform_workbench.go:1674,1680,1691,1699,1703` — 其余 5 处 fallback/stream 日志
- `internal/workbench/console_platform_workbench_test.go:60,118,164,192,201,228,248,273,283,296,324,357,381` — 全部传入 `nil` logger

## Required outcome
- 至少一个测试注入 buffer-backed `*slog.Logger`(如 `slog.NewJSONHandler`),触发 generate_prompt fallback 路径,解析 JSON 行并断言 `msg`/`component`(若有)/`reason` 字段。
-`internal/messages/handler_test.go:23``TestHandlerUsesInjectedLogger`)和 `internal/webhooks/enqueuer_test.go``TestEnqueuerUsesOwnedLogger`)保持一致风格。

## Suggested approach
参照 `internal/messages/handler_test.go:23-46``TestHandlerUsesInjectedLogger`:构造 `slog.New(slog.NewJSONHandler(&buf, nil)).With("component", "workbench")`,传入 `newWorkbenchHandler`,发一个会命中 fallback 的请求(如 marshal 失败或无 upstream token),然后 `json.Unmarshal` 并校验字段。

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using anthropic/glm-5.2𝕏

upstreamReq.Header.Set("Anthropic-Version", anthropicAPIVersion)

log.Printf("workbench generate_prompt upstream_start org=%s endpoint=%s model=%s task_chars=%d thinking=%t", chi.URLParam(r, "orgUUID"), endpoint, workbenchGeneratePromptModel(), len([]rune(task)), payload.TargetThinkingMode)
h.logger.InfoContext(r.Context(), "workbench generate prompt upstream start", "organization_uuid", organizationUUID, "upstream_host", upstreamReq.URL.Host, "model", workbenchGeneratePromptModel(), "task_chars", len([]rune(task)), "thinking", payload.TargetThinkingMode)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 这里新增了 upstream_host 字段(取 upstreamReq.URL.Host,按构造只含 host),但 console_platform_workbench_test.go 把所有测试都迁移到了 newWorkbenchHandler(..., nil) 且没有对日志做任何断言。本 PR 在 messages/webhooks 里都加了 JSONHandler + 结构化字段断言的 logger 注入测试,workbench 作为改动量最大的 handler 反而缺这一层。建议补一个同形测试覆盖 fallback 路径并顺带验证 upstream_host 不含完整 URL/凭据。详见 review body 的 Technical details。

t.Fatalf("response status = %d, want %d", response.Code, http.StatusOK)
}
for _, want := range []string{"stream platform file preview failed", "file_uuid=file-uuid", "key=object-key", "bytes_copied=0", "expected_size=4", readErr.Error()} {
for _, want := range []string{"stream platform file failed", "variant=preview", "file_uuid=file-uuid", "key=object-key", "bytes_copied=0", "expected_size=4", readErr.Error()} {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 这里用 strings.Contains(logOutput, want)TextHandler 渲染出的 key=value 子串做断言,甚至匹配 readErr.Error()——正是本 PR 自己加进 AGENTS.md 的日志规范所提醒的 "不要依赖整行文本格式"。如果错误消息含空格或引号,TextHandler 的引号转义会让子串匹配失败。同 PR 的 internal/api/http_log_test.goJSONHandler + 解析 map 的做法更稳,建议这里也对齐。


line := stripANSI(strings.TrimSpace(buf.String()))
if !strings.Contains(line, " [api] GET 200 12.3ms /v1/files?beta=true ") {
if !strings.Contains(line, " [api] GET 200 12.3ms /v1/files ") {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 本 PR 已经在编辑这个测试(消息和 URL 都改了),但仍保留 strings.Contains(line, " [api] GET 200 12.3ms /v1/files ") 这种依赖 ConsoleHandler 整行文本格式的断言,且没有独立检查 record 的 level。既然已经动了这几行,顺手转成结构化 record 断言(解析 attrs、分别校验 level/msg)成本最低。

Comment thread internal/skillprewarm/worker_test.go Outdated
}},
}
worker := NewWorker(store, store, store, &fakeResolver{err: errors.New("resolve failed")}, &fakePreparer{})
worker := NewWorker(store, store, store, &fakeResolver{err: errors.New("resolve failed")}, &fakePreparer{}, nil)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 这里(以及 otlp_file_log_test.go:42,154)传入 nil logger,经 LoggerOrDefault 回落到 slog.Default(),于是每次跑到失败路径都会向测试进程的 stderr 打 ERROR 级结构化日志。不是 bug,但本 PR 在 messages/webhooks/files 里都改成了注入 buffer/discard logger,这里还沿用 nil 会留下测试输出噪音。建议统一注入 slog.New(slog.NewTextHandler(io.Discard, nil)) 或捕获后断言。

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

该文件随 internal/skillprewarm 包整体删除(合并 PR #160 后已不再存在),nil logger 噪音问题随之消失。

Pullfrog  | View workflow run | via Pullfrog | Using anthropic/glm-5.2𝕏

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (6)
internal/workbench/console_platform_workbench.go (2)

1952-1979: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Multi-record writes not wrapped in a transaction.

storeRevision upserts a WorkbenchRevisionRecord and then separately upserts the WorkbenchPromptRecord (latest revision pointer); storeKV upserts the prompt record and then the KV record. Each is two independent store calls — if the second call fails after the first succeeds, the prompt and its revision/KV state can diverge (e.g., a revision persisted without the prompt's latest_revision_uuid being updated, or vice versa).

As per coding guidelines for internal/**/*.go: "多表写入、状态机推进、幂等写入和 outbox/event 写入必须保持事务一致性;不要把同一事务拆散到多个 handler 分支。" Consider exposing a single atomic persistence operation (e.g., a store method that upserts both records in one DB transaction) for these two-step sequences.

Also applies to: 2214-2239

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/workbench/console_platform_workbench.go` around lines 1952 - 1979,
Make the multi-record persistence in storeRevision and the corresponding storeKV
flow atomic by introducing or reusing a store operation that upserts the related
WorkbenchRevisionRecord/WorkbenchPromptRecord or prompt/KV records within one
database transaction. Replace the separate store.UpsertWorkbenchRevision,
store.UpsertWorkbenchPrompt, and KV/prompt calls with that atomic operation
while preserving existing record contents and local-cache updates only after
persistence succeeds.

Source: Coding guidelines


1644-1704: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use the registered org UUID param key.

In internal/workbench/console_platform_workbench.go:1660, reading chi.URLParam(r, "orgUUID") misses the registered "orgUuid" route parameter used elsewhere in this package. organizationUUID will be empty in the handler’s log attributes; use the existing workbenchOrgUUID(r) helper or trim chi.URLParam(r, "orgUuid").

🐛 Proposed fix
-	organizationUUID := chi.URLParam(r, "orgUUID")
+	organizationUUID := workbenchOrgUUID(r)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/workbench/console_platform_workbench.go` around lines 1644 - 1704,
Update organizationUUID initialization in handleWorkbenchGeneratePrompt to use
the registered orgUuid route parameter, preferably via the existing
workbenchOrgUUID helper, so all subsequent log attributes contain the
organization UUID.
internal/api/http_log_test.go (1)

42-50: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use snake_case for HTTP log attributes.

internal/api/http_log.go emits requestId, clientKind, anthropicClientPlatform, and anthropicClientApp, and the test asserts the same names; rename them to request_id, client_kind, anthropic_client_platform, anthropic_client_app, and duration_ms consistently.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/http_log_test.go` around lines 42 - 50, Rename the HTTP log
attributes emitted by the request logging implementation and asserted in the
test from camelCase to snake_case: requestId to request_id, clientKind to
client_kind, anthropicClientPlatform to anthropic_client_platform,
anthropicClientApp to anthropic_client_app, and durationMs to duration_ms.
Update the relevant logging code in http_log.go and matching assertions in
http_log_test.go consistently.

Source: Coding guidelines

internal/memory/handler.go (1)

1745-1808: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Include resource IDs in memory operation failure records.

The storeID, memoryID, and versionID parameters are available, but these error paths only log error. Add explicit attributes such as memory_store_id, memory_id, and memory_version_id, preserve the request context in the log call, and add a structured logging test covering the new fields and message.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/memory/handler.go` around lines 1745 - 1808, Update
writeStoreLoadError, writeMemoryLoadError, writeVersionLoadError, and
writeMemoryMutationError to include their available resource IDs as structured
attributes (memory_store_id, memory_id, and memory_version_id) in failure logs
while preserving the request context; add a structured logging test that
verifies the expected message and fields.

Source: Coding guidelines

internal/api/http_log.go (1)

24-40: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use snake_case for structured HTTP attributes.

These records now emit requestId, durationMs, userAgent, clientKind, and Anthropic fields as attributes. Rename them to request_id, duration_ms, user_agent, client_kind, etc.

As per coding guidelines, “新增属性名使用 snake_case”.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/http_log.go` around lines 24 - 40, Update the structured
attribute names produced by httpLogAttrs and the response attributes in the HTTP
logging middleware to use snake_case, including request_id, duration_ms,
user_agent, client_kind, and all Anthropic-related fields. Preserve the existing
attribute values and logging behavior.

Source: Coding guidelines

internal/api/server.go (1)

84-142: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Run the required Go checks in a PR environment before merge.

internal/api/server.go is Go handler/routing code, so run just dead-code, just lint, just duplicates, and go test ./... -count=1; the PR summary should not say tests were skipped.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/server.go` around lines 84 - 142, Run just dead-code, just lint,
just duplicates, and go test ./... -count=1 for the NewServer routing and
handler changes, then address any reported issues before merging. Ensure the PR
summary records the checks and does not state that tests were skipped.

Source: Coding guidelines

🧹 Nitpick comments (6)
internal/workbench/workbench_handler.go (1)

38-44: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Silent fallback when store doesn't satisfy workbenchPersistenceStore.

workbenchPersistenceFromStore type-asserts store to workbenchPersistenceStore and discards the ok result. If the concrete store passed to RegisterOrgWorkbenchRoutes ever fails to implement the full persistence interface (e.g., after an unrelated interface change), h.store becomes nil and the entire workbench feature silently falls back to process-local, non-persistent maps — with no log signal that persistence was disabled.

Consider logging a warning (via the injected logger) when the assertion fails, so a misconfiguration is observable instead of silently degrading durability.

Also applies to: 83-86

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/workbench/workbench_handler.go` around lines 38 - 44, The workbench
persistence adapter silently falls back when the store does not implement
workbenchPersistenceStore. Update workbenchPersistenceFromStore and its use in
newWorkbenchHandler to detect a failed type assertion and emit a warning through
the injected logger, while preserving the existing fallback behavior and normal
path when the assertion succeeds.
internal/skills/handler.go (2)

163-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use context-aware slog methods throughout these paths.

r.Context()/ctx is already available, but these new records call Error. Use ErrorContext so request and worker context reaches context-aware handlers; apply this to the remaining changed logs in each file.

  • internal/skills/handler.go#L163-L163: use h.logger.ErrorContext(r.Context(), ...).
  • internal/skills/seed.go#L107-L107: use logger.ErrorContext(ctx, ...).
  • internal/vaults/handler.go#L194-L194: use h.logger.ErrorContext(r.Context(), ...).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/skills/handler.go` at line 163, Replace the changed Error calls with
context-aware ErrorContext calls, passing r.Context() in
internal/skills/handler.go:163-163 and internal/vaults/handler.go:194-194, and
ctx in internal/skills/seed.go:107-107. Preserve each existing log message and
attributes.

Source: Coding guidelines


730-748: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use an explicit object-key attribute name.

key is ambiguous in object-store records. Rename it to object_key here and in the remaining changed object-storage logs.

  • internal/skills/handler.go#L730-L748: replace key with object_key.
  • internal/skills/seed.go#L123-L126: replace key with object_key.

As per coding guidelines, resource identifiers must use explicit fields and new attribute names must use snake_case.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/skills/handler.go` around lines 730 - 748, The object-storage logs
use the ambiguous attribute name key; rename it to object_key in the affected
logs in internal/skills/handler.go lines 730-748 and internal/skills/seed.go
lines 123-126, including the stream, open, and seed-related logging calls as
applicable. No other log attributes or behavior need changing.

Source: Coding guidelines

internal/codesessions/ingress.go (1)

865-910: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor duplication between the two request-rejection loggers.

logCodeSessionWorkerInternalEventsBadRequest and logCodeSessionWorkerOTLPRequest repeat the same path extraction and several common fields. Could be worth factoring a shared helper for the common request metadata.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/codesessions/ingress.go` around lines 865 - 910, Factor the repeated
request metadata from logCodeSessionWorkerInternalEventsBadRequest and
logCodeSessionWorkerOTLPRequest into a shared helper or reusable field
construction, including path extraction and common request fields. Keep each
logger’s event-specific fields—such as signal, epoch details, reason, and
error—unchanged while preserving the existing log output.
internal/files/platform_test.go (1)

26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert structured log records, not formatted text.

These substring assertions couple the tests to TextHandler rendering. Capture slog.Record values and assert level, message, and attrs such as variant, file_uuid, and expected_size.

As per coding guidelines, new public log fields should have record tests asserting level, message, and attrs.

Also applies to: 47-54, 106-110

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/files/platform_test.go` around lines 26 - 33, Update the platform
logging tests around capturePlatformLog and the cases at the referenced failure
paths to capture slog.Record values instead of rendered log text. Assert each
record’s level, message, and structured attributes—including variant, file_uuid,
key, bytes_copied, and expected_size—while preserving the existing error
scenarios and readErr details where applicable.

Source: Coding guidelines

internal/codesessions/service.go (1)

54-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Preserve request and worker context in migrated logs.

The new injected loggers are used without ctx/r.Context(), losing context-derived fields such as request IDs.

  • internal/codesessions/service.go#L54-L62: use WarnContext/ErrorContext for Lines 54, 62, 97, and 359.
  • internal/codesessions/tool_permissions.go#L37-L40: use ErrorContext and InfoContext.
  • internal/environments/runner.go#L78-L78: use ErrorContext(ctx, ...).
  • internal/files/platform.go#L139-L144: use context-aware methods for the changed request-handler logs through Line 250.
  • internal/sessions/code_event_bridge.go#L18-L18: use context-aware methods for Lines 18, 40, and 48.
  • internal/sessions/service.go#L52-L52: use context-aware methods for the changed request-handler logs through Line 792.

As per coding guidelines, paths with context.Context should prefer DebugContext, InfoContext, WarnContext, and ErrorContext.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/codesessions/service.go` around lines 54 - 62, Preserve
context-derived fields by replacing the affected injected logger calls with
their context-aware variants, passing the available request or worker context.
Apply this to internal/codesessions/service.go lines 54-62, 97, and 359;
internal/codesessions/tool_permissions.go lines 37-40;
internal/environments/runner.go line 78 using ctx; internal/files/platform.go
changed request-handler logs through line 250;
internal/sessions/code_event_bridge.go lines 18, 40, and 48; and
internal/sessions/service.go changed request-handler logs through line 792. Use
DebugContext, InfoContext, WarnContext, or ErrorContext matching each existing
log level.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/seed-builtin-skills/main.go`:
- Around line 70-74: Update the seed-result reporting after the import operation
to use the injected structured logger instead of the three fmt.Printf calls. Log
the imported count, optional pruned count, and result.Skills as separate
structured fields while preserving the existing message content and conditional
pruned reporting.

In `@internal/agents/handler.go`:
- Line 174: Preserve context and resource scope in every listed structured
failure log: in internal/agents/handler.go lines 174, 219, 261, 302, 341, 380,
406, and 444, replace logger Error calls with ErrorContext(r.Context(), ...) and
add the specified agent_id or workspace_id; update lines 1237-1243 to use
ErrorContext(ctx, ...). In internal/skillprewarm/worker.go lines 98 and 194, use
ErrorContext(ctx, ...).

In `@internal/api/http_log_test.go`:
- Around line 58-60: Strengthen the assertion in the HTTP logging test around
the buffer check so it verifies that no query values are logged, including both
“secret-state” and “beta=true”. Prefer rejecting the request’s complete RawQuery
representation when validating the no-query logging contract.

In `@internal/api/platform_mcp_vault_auth.go`:
- Around line 201-203: Sanitize all OAuth URL/query-derived values before
logging in internal/api/platform_mcp_vault_auth.go: at lines 201-203, replace
the raw discovery error with allow-listed metadata such as host and HTTP status;
at lines 337-339, omit the callback’s raw error query value and log only a fixed
event and flow ID; at lines 482-488, similarly sanitize protected-resource
metadata fetch errors before attaching them to logs. Ensure no complete query
strings, OAuth codes/state, credentials, or credential payloads are recorded.

In `@internal/api/server.go`:
- Around line 277-283: Update the panic-recovery logging around the ErrorContext
call to stop recording the arbitrary recovered panic value, which may contain
sensitive request-derived data. Preserve logging of request_id and stack, and
remove the "panic" field or replace it only with an approved sanitized
representation.

In `@internal/codesessions/ingress.go`:
- Around line 756-763: Update internal/codesessions/ingress.go lines 756-763:
extend Handler.writeIngressLoadError with the available codeSessionID parameter,
update all callers to pass it, and include it as "code_session_id" in the
internal-error log. Update internal/sessions/event_payload.go lines 418-429,
431-438, and 440-447: add "session_id", "thread_id", and "resource_id"
respectively to the internal-error logs in writeSessionLoadError,
writeThreadLoadError, and writeResourceLoadError, reusing their existing
identifier parameters.
- Around line 55-57: Replace contextless structured logging with the
context-aware slog methods wherever context is available. In
internal/codesessions/ingress.go at 55-57, 65-67, 77-79, 110-112, 172-180,
201-203, 251-253, 305-313, 328-330, 367-369, 398-400, 433-435, 460-462, 474-476,
535-537, 563-565, 595-597, 760-762, and 836-838, use ErrorContext or WarnContext
with r.Context(), or the existing ctx parameter in
streamCodeSessionWorkerEvents; in internal/sessions/event_effects.go at 23-31,
46-52, 63-70, 86, 99, and 113, use ErrorContext(ctx, ...); in
internal/sessions/event_payload.go at 400-416, 418-429, 431-438, and 440-447,
use ErrorContext(r.Context(), ...); and in
internal/codesessions/otlp_file_log.go at 126-128, use ErrorContext(r.Context(),
...), preserving each existing log message and attributes.

In `@internal/codesessions/service.go`:
- Around line 37-43: Run go test ./... -count=1 and fix the failures in
internal/workbench and the Postgres-backed test suite. Update the affected test
setup and callers of NewServiceWithCredentials to provide valid
SessionCredentials and required Postgres configuration, preserving the
constructor’s non-nil credential contract. Verify the full test suite passes.

In `@internal/filestore/handler.go`:
- Around line 94-100: The filestore panic recovery log must not include the raw
recovered value. In the panic handler containing the “filestore panic recovered”
message, remove the “panic” attribute while preserving the request ID and stack
fields, and add a structured-record test that verifies a sensitive panic value
is absent from the emitted log.

In `@internal/sessions/transport.go`:
- Around line 18-21: Change NewHandler to return (*Handler, error) and replace
the nil codeSessionService panic with a descriptive construction error. Update
every caller, especially the executable startup boundary, to propagate or handle
the returned error so the composition layer can log the terminal startup failure
and choose the exit code.

In `@internal/workbench/workbench_handler.go`:
- Line 62: Update the POST revision rename route to call the dedicated revision
rename handler instead of handleGetWorkbenchPromptRevision. Preserve the
existing route path and method, and use the handler symbol already defined for
renaming revisions.

In `@main.go`:
- Line 95: Guard the batches.NewWorker(...).Start(ctx) call with
cfg.Batch.WorkerEnabled so batch processing starts only when the worker is
enabled. Preserve the existing worker construction and logger configuration when
the gate is true.

In `@tests/files_api_test.go`:
- Around line 452-454: Move the capture logger setup in the test before
constructing fakeApp, preserving the original logger for deferred restoration.
Ensure api.NewServer receives and scopes the capture logger so the logs buffer
records the expected stream-error entry.

---

Outside diff comments:
In `@internal/api/http_log_test.go`:
- Around line 42-50: Rename the HTTP log attributes emitted by the request
logging implementation and asserted in the test from camelCase to snake_case:
requestId to request_id, clientKind to client_kind, anthropicClientPlatform to
anthropic_client_platform, anthropicClientApp to anthropic_client_app, and
durationMs to duration_ms. Update the relevant logging code in http_log.go and
matching assertions in http_log_test.go consistently.

In `@internal/api/http_log.go`:
- Around line 24-40: Update the structured attribute names produced by
httpLogAttrs and the response attributes in the HTTP logging middleware to use
snake_case, including request_id, duration_ms, user_agent, client_kind, and all
Anthropic-related fields. Preserve the existing attribute values and logging
behavior.

In `@internal/api/server.go`:
- Around line 84-142: Run just dead-code, just lint, just duplicates, and go
test ./... -count=1 for the NewServer routing and handler changes, then address
any reported issues before merging. Ensure the PR summary records the checks and
does not state that tests were skipped.

In `@internal/memory/handler.go`:
- Around line 1745-1808: Update writeStoreLoadError, writeMemoryLoadError,
writeVersionLoadError, and writeMemoryMutationError to include their available
resource IDs as structured attributes (memory_store_id, memory_id, and
memory_version_id) in failure logs while preserving the request context; add a
structured logging test that verifies the expected message and fields.

In `@internal/workbench/console_platform_workbench.go`:
- Around line 1952-1979: Make the multi-record persistence in storeRevision and
the corresponding storeKV flow atomic by introducing or reusing a store
operation that upserts the related WorkbenchRevisionRecord/WorkbenchPromptRecord
or prompt/KV records within one database transaction. Replace the separate
store.UpsertWorkbenchRevision, store.UpsertWorkbenchPrompt, and KV/prompt calls
with that atomic operation while preserving existing record contents and
local-cache updates only after persistence succeeds.
- Around line 1644-1704: Update organizationUUID initialization in
handleWorkbenchGeneratePrompt to use the registered orgUuid route parameter,
preferably via the existing workbenchOrgUUID helper, so all subsequent log
attributes contain the organization UUID.

---

Nitpick comments:
In `@internal/codesessions/ingress.go`:
- Around line 865-910: Factor the repeated request metadata from
logCodeSessionWorkerInternalEventsBadRequest and logCodeSessionWorkerOTLPRequest
into a shared helper or reusable field construction, including path extraction
and common request fields. Keep each logger’s event-specific fields—such as
signal, epoch details, reason, and error—unchanged while preserving the existing
log output.

In `@internal/codesessions/service.go`:
- Around line 54-62: Preserve context-derived fields by replacing the affected
injected logger calls with their context-aware variants, passing the available
request or worker context. Apply this to internal/codesessions/service.go lines
54-62, 97, and 359; internal/codesessions/tool_permissions.go lines 37-40;
internal/environments/runner.go line 78 using ctx; internal/files/platform.go
changed request-handler logs through line 250;
internal/sessions/code_event_bridge.go lines 18, 40, and 48; and
internal/sessions/service.go changed request-handler logs through line 792. Use
DebugContext, InfoContext, WarnContext, or ErrorContext matching each existing
log level.

In `@internal/files/platform_test.go`:
- Around line 26-33: Update the platform logging tests around capturePlatformLog
and the cases at the referenced failure paths to capture slog.Record values
instead of rendered log text. Assert each record’s level, message, and
structured attributes—including variant, file_uuid, key, bytes_copied, and
expected_size—while preserving the existing error scenarios and readErr details
where applicable.

In `@internal/skills/handler.go`:
- Line 163: Replace the changed Error calls with context-aware ErrorContext
calls, passing r.Context() in internal/skills/handler.go:163-163 and
internal/vaults/handler.go:194-194, and ctx in internal/skills/seed.go:107-107.
Preserve each existing log message and attributes.
- Around line 730-748: The object-storage logs use the ambiguous attribute name
key; rename it to object_key in the affected logs in internal/skills/handler.go
lines 730-748 and internal/skills/seed.go lines 123-126, including the stream,
open, and seed-related logging calls as applicable. No other log attributes or
behavior need changing.

In `@internal/workbench/workbench_handler.go`:
- Around line 38-44: The workbench persistence adapter silently falls back when
the store does not implement workbenchPersistenceStore. Update
workbenchPersistenceFromStore and its use in newWorkbenchHandler to detect a
failed type assertion and emit a warning through the injected logger, while
preserving the existing fallback behavior and normal path when the assertion
succeeds.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: afd10e47-11eb-44e3-9d97-898d367af0b9

📥 Commits

Reviewing files that changed from the base of the PR and between 41cbca5 and 5b78111.

📒 Files selected for processing (76)
  • .gitignore
  • AGENTS.md
  • cmd/migrate/main.go
  • cmd/seed-builtin-skills/main.go
  • docs/design/be/ccrv2/otlp-metrics-api.md
  • docs/design/be/runtime-configuration.md
  • internal/admin/handler.go
  • internal/agents/handler.go
  • internal/api/filestore_auth_test.go
  • internal/api/http_log.go
  • internal/api/http_log_test.go
  • internal/api/platform_mcp_vault_auth.go
  • internal/api/platform_mcp_vault_auth_log_test.go
  • internal/api/server.go
  • internal/api/service_auth.go
  • internal/batches/handler.go
  • internal/batches/worker.go
  • internal/cleanup/worker.go
  • internal/codesessions/handler.go
  • internal/codesessions/ingress.go
  • internal/codesessions/otlp_file_log.go
  • internal/codesessions/otlp_file_log_test.go
  • internal/codesessions/service.go
  • internal/codesessions/service_test_helpers_test.go
  • internal/codesessions/session_credentials_test.go
  • internal/codesessions/tool_permissions.go
  • internal/codesessions/upstream_proxy_mitm_test.go
  • internal/codesessions/upstream_proxy_policy.go
  • internal/codesessions/upstream_proxy_policy_test.go
  • internal/codesessions/upstream_proxy_test.go
  • internal/deployments/handler.go
  • internal/environments/handler.go
  • internal/environments/runner.go
  • internal/files/handler.go
  • internal/files/platform.go
  • internal/files/platform_test.go
  • internal/filestore/cleanup.go
  • internal/filestore/handler.go
  • internal/filestore/handler_test.go
  • internal/logging/logging.go
  • internal/logging/logging_test.go
  • internal/mcpcatalogs/handler.go
  • internal/memory/handler.go
  • internal/messages/handler.go
  • internal/messages/handler_test.go
  • internal/sessions/access.go
  • internal/sessions/code_event_bridge.go
  • internal/sessions/event_effects.go
  • internal/sessions/event_payload.go
  • internal/sessions/handler.go
  • internal/sessions/service.go
  • internal/sessions/stream_hub.go
  • internal/sessions/transport.go
  • internal/sessions/webhook_bridge.go
  • internal/skillprewarm/worker.go
  • internal/skillprewarm/worker_test.go
  • internal/skills/handler.go
  • internal/skills/seed.go
  • internal/vaults/handler.go
  • internal/webhooks/enqueuer.go
  • internal/webhooks/enqueuer_test.go
  • internal/webhooks/handler.go
  • internal/webhooks/webhooks.go
  • internal/workbench/console_platform_workbench.go
  • internal/workbench/console_platform_workbench_test.go
  • internal/workbench/workbench_handler.go
  • internal/workbench/workbench_support.go
  • main.go
  • tests/environments_e2b_integration_test.go
  • tests/environments_full_e2b_bridge_integration_test.go
  • tests/environments_runner_cloud_test.go
  • tests/files_api_test.go
  • tests/sdk_go_quickstart_demo_test.go
  • tests/sessions_api_test.go
  • tests/skills_seed_test.go
  • tests/webhooks_api_test.go

Comment on lines 70 to 74
fmt.Printf("Imported %d builtin skill(s)", result.Imported)
if result.Pruned > 0 {
fmt.Printf(", pruned %d version(s)", result.Pruned)
}
fmt.Printf(": %v\n", result.Skills)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Emit the seed result through the injected logger.

These operational messages bypass the structured logger and serialize dynamic values into a formatted string.

Proposed fix
-	fmt.Printf("Imported %d builtin skill(s)", result.Imported)
-	if result.Pruned > 0 {
-		fmt.Printf(", pruned %d version(s)", result.Pruned)
-	}
-	fmt.Printf(": %v\n", result.Skills)
+	logger.Info("builtin skills seeded",
+		"imported", result.Imported,
+		"pruned", result.Pruned,
+		"skill_ids", result.Skills,
+	)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fmt.Printf("Imported %d builtin skill(s)", result.Imported)
if result.Pruned > 0 {
fmt.Printf(", pruned %d version(s)", result.Pruned)
}
fmt.Printf(": %v\n", result.Skills)
logger.Info("builtin skills seeded",
"imported", result.Imported,
"pruned", result.Pruned,
"skill_ids", result.Skills,
)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/seed-builtin-skills/main.go` around lines 70 - 74, Update the seed-result
reporting after the import operation to use the injected structured logger
instead of the three fmt.Printf calls. Log the imported count, optional pruned
count, and result.Skills as separate structured fields while preserving the
existing message content and conditional pruned reporting.

Source: Coding guidelines

Comment thread internal/agents/handler.go Outdated
Comment on lines +58 to +60
if strings.Contains(buf.String(), "secret-state") {
t.Fatalf("request query leaked into logs: %s", buf.String())
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Assert that every query value is absent.

The test rejects secret-state but permits leaking beta=true through a separate query attribute. Check both values, or reject req.URL.RawQuery, so the test enforces the no-query logging contract.

Suggested test tightening
-if strings.Contains(buf.String(), "secret-state") {
+if strings.Contains(buf.String(), "beta=true") ||
+    strings.Contains(buf.String(), "secret-state") {
     t.Fatalf("request query leaked into logs: %s", buf.String())
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if strings.Contains(buf.String(), "secret-state") {
t.Fatalf("request query leaked into logs: %s", buf.String())
}
if strings.Contains(buf.String(), "beta=true") ||
strings.Contains(buf.String(), "secret-state") {
t.Fatalf("request query leaked into logs: %s", buf.String())
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/http_log_test.go` around lines 58 - 60, Strengthen the assertion
in the HTTP logging test around the buffer check so it verifies that no query
values are logged, including both “secret-state” and “beta=true”. Prefer
rejecting the request’s complete RawQuery representation when validating the
no-query logging contract.

Source: Coding guidelines

Comment on lines +201 to +203
discovery, err := s.discoverPlatformMCPOAuth(r.Context(), platformMCPVaultAuthHTTPClient, mcpServerURL)
if err != nil {
log.Printf("discover mcp oauth for %s: %v", mcpServerURL, err)
s.logger.ErrorContext(r.Context(), "discover mcp oauth", "mcp_server_host", platformMCPLogHost(mcpServerURL), "error", err)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Sanitize OAuth URL/query-derived log values.

Discovery errors can embed metadataURL (including query data) and providerError is read directly from the callback query string. Do not attach either raw value to logs; record only allow-listed metadata such as host, HTTP status, and a validated OAuth error code.

  • internal/api/platform_mcp_vault_auth.go#L201-L203: sanitize URL-bearing discovery errors before attaching them as error.
  • internal/api/platform_mcp_vault_auth.go#L337-L339: omit the raw error query value; log a fixed event and flow ID only.
  • internal/api/platform_mcp_vault_auth.go#L482-L488: sanitize protected-resource metadata fetch errors before logging.

As per coding guidelines, application logs must not record complete query strings, OAuth codes/state, credentials, or credential payloads.

📍 Affects 1 file
  • internal/api/platform_mcp_vault_auth.go#L201-L203 (this comment)
  • internal/api/platform_mcp_vault_auth.go#L337-L339
  • internal/api/platform_mcp_vault_auth.go#L482-L488
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/platform_mcp_vault_auth.go` around lines 201 - 203, Sanitize all
OAuth URL/query-derived values before logging in
internal/api/platform_mcp_vault_auth.go: at lines 201-203, replace the raw
discovery error with allow-listed metadata such as host and HTTP status; at
lines 337-339, omit the callback’s raw error query value and log only a fixed
event and flow ID; at lines 482-488, similarly sanitize protected-resource
metadata fetch errors before attaching them to logs. Ensure no complete query
strings, OAuth codes/state, credentials, or credential payloads are recorded.

Source: Coding guidelines

Comment thread internal/api/server.go
Comment on lines +277 to +283
s.logger.ErrorContext(
r.Context(),
"panic recovered",
"request_id", requestID,
"panic", recovered,
"stack", string(debug.Stack()),
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Do not log the arbitrary panic value.

recovered can contain request-derived data. Keep request_id and stack, but omit or sanitize "panic" before recording it.

As per coding guidelines, panic recovery must record request_id and stack “但不得包含敏感 payload”.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/server.go` around lines 277 - 283, Update the panic-recovery
logging around the ErrorContext call to stop recording the arbitrary recovered
panic value, which may contain sensitive request-derived data. Preserve logging
of request_id and stack, and remove the "panic" field or replace it only with an
approved sanitized representation.

Source: Coding guidelines

Comment on lines +94 to +100
h.logger.ErrorContext(
r.Context(),
"filestore panic recovered",
"request_id", httpapi.RequestID(r.Context()),
"panic", recovered,
"stack", string(debug.Stack()),
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not log the raw recovered panic value.

recovered can contain request-derived content, credentials, or body fragments. Keep the request ID and stack, but remove or strictly redact the panic attribute; add a structured-record test proving a sensitive panic value is absent.

Suggested fix
 h.logger.ErrorContext(
     r.Context(),
     "filestore panic recovered",
     "request_id", httpapi.RequestID(r.Context()),
-    "panic", recovered,
     "stack", string(debug.Stack()),
 )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
h.logger.ErrorContext(
r.Context(),
"filestore panic recovered",
"request_id", httpapi.RequestID(r.Context()),
"panic", recovered,
"stack", string(debug.Stack()),
)
h.logger.ErrorContext(
r.Context(),
"filestore panic recovered",
"request_id", httpapi.RequestID(r.Context()),
"stack", string(debug.Stack()),
)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/filestore/handler.go` around lines 94 - 100, The filestore panic
recovery log must not include the raw recovered value. In the panic handler
containing the “filestore panic recovered” message, remove the “panic” attribute
while preserving the request ID and stack fields, and add a structured-record
test that verifies a sensitive panic value is absent from the emitted log.

Source: Coding guidelines

Comment on lines +18 to 21
func NewHandler(cfg config.Config, database *db.DB, codeSessionService *codesessions.Service, webhookEvents webhookEnqueuer, logger *slog.Logger) *Handler {
if codeSessionService == nil {
panic("sessions: code-session service is required")
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Return a construction error instead of panicking.

A missing injected service crashes the process rather than allowing the composition layer to log one terminal startup error and choose the exit code. Change NewHandler to return an error and propagate it to the executable boundary.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/sessions/transport.go` around lines 18 - 21, Change NewHandler to
return (*Handler, error) and replace the nil codeSessionService panic with a
descriptive construction error. Update every caller, especially the executable
startup boundary, to propagate or handle the returned error so the composition
layer can log the terminal startup failure and choose the exit code.

Source: Coding guidelines

r.Get("/workbench/prompts/{promptUuid}/revisions", h.handleListWorkbenchPromptRevisions)
r.Post("/workbench/prompts/{promptUuid}/revisions", h.handleCreateWorkbenchPromptRevision)
r.Get("/workbench/prompts/{promptUuid}/revisions/{revisionUuid}", h.handleGetWorkbenchPromptRevision)
r.Post("/workbench/prompts/{promptUuid}/revisions/{revisionUuid}/rename", h.handleGetWorkbenchPromptRevision)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n -i 'rename' internal/workbench

Repository: superduck-ai/open-managed-agents

Length of output: 468


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== handler outline =="
ast-grep outline internal/workbench/workbench_handler.go --view compact 2>/dev/null || ast-grep outline internal/workbench/workbench_handler.go || true

echo
echo "== workbench handler relevant lines =="
cat -n internal/workbench/workbench_handler.go | sed -n '1,180p'

echo
echo "== search handleGetWorkbenchPromptRevision usages =="
rg -n -i 'handleGetWorkbenchPromptRevision|GetWorkbenchPromptRevision|revision.*rename|rename.*revision' internal/workbench . --glob '!vendor/**' --glob '!node_modules/**'

Repository: superduck-ai/open-managed-agents

Length of output: 50391


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== handleGetWorkbenchPromptRevision definition =="
cat -n internal/workbench/console_platform_workbench.go | sed -n '290,340p'

echo
echo "== route-related rename search, excluding generated quickstart docs =="
rg -n --fixed-strings 'rename' internal/workbench web --glob '!web/src/features/managed-agents/quickstart/platformQuickstartOfficialRequest.generated.ts' || true

echo
echo "== route references, excluding generated quickstart docs =="
rg -n --fixed-strings '/revisions/{}/rename|/revisions/{}/rename|/revisions/{revisionUuid}/rename|revisionUuid}/rename' internal web --glob '!web/src/features/managed-agents/quickstart/platformQuickstartOfficialRequest.generated.ts' || true

Repository: superduck-ai/open-managed-agents

Length of output: 6067


Wire a rename handler for the revision rename route.

POST /workbench/prompts/{promptUuid}/revisions/{revisionUuid}/rename currently calls handleGetWorkbenchPromptRevision, a read-only revision fetch. The Web page exercises a revision rename flow, so this route should point to a dedicated rename handler rather than reuse the GET revision logic.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/workbench/workbench_handler.go` at line 62, Update the POST revision
rename route to call the dedicated revision rename handler instead of
handleGetWorkbenchPromptRevision. Preserve the existing route path and method,
and use the handler symbol already defined for renaming revisions.

Comment thread main.go Outdated
skillprewarm.StartWorker(ctx, database, objectStore, cfg)
webhooks.StartWorker(ctx, database, cfg.Webhook)
filestore.NewCleanupWorker(database, storageClient, logger.With("component", "filestore_cleanup")).Start(ctx)
batches.NewWorker(database, objectStore, cfg, logger.With("component", "batches")).Start(ctx)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Preserve the batch worker enablement gate.

Line 95 starts batch processing even when cfg.Batch.WorkerEnabled is false, changing an operational opt-out into active job execution.

Proposed fix
- batches.NewWorker(database, objectStore, cfg, logger.With("component", "batches")).Start(ctx)
+ if cfg.Batch.WorkerEnabled {
+     batches.NewWorker(database, objectStore, cfg, logger.With("component", "batches")).Start(ctx)
+ }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
batches.NewWorker(database, objectStore, cfg, logger.With("component", "batches")).Start(ctx)
if cfg.Batch.WorkerEnabled {
batches.NewWorker(database, objectStore, cfg, logger.With("component", "batches")).Start(ctx)
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@main.go` at line 95, Guard the batches.NewWorker(...).Start(ctx) call with
cfg.Batch.WorkerEnabled so batch processing starts only when the worker is
enabled. Preserve the existing worker construction and logger configuration when
the gate is true.

Comment thread tests/files_api_test.go Outdated

@duckpr duckpr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DuckPR reviewer: opencode
Model: anthropic/glm-5.2

✅ No new issues found.

Reviewed changes — 本次增量评审覆盖自上次 pullfrog 评审(5b78111)以来的 1 个新提交(4ab58d4),主要将大量 handler/worker 的结构化日志调用从无 context 变体迁移到 *Context 变体,并引入 AST 静态检查防止回退。

  • 批量迁移到 context-aware slog 方法 — 将 agentsbatchescodesessionsdeploymentsenvironmentsfilesmemorymessagessessionsskillsvaultsapi 等 handler 中约 100 处 h.logger.Error(...) / Warn(...) / Info(...) 统一替换为 ErrorContext(r.Context(), ...) / WarnContext(...) / InfoContext(...),确保 request ID 和 trace 字段能随请求上下文传播。worker 路径(batchescleanupskillprewarmcodesessions service)同样改用 ctx 变体。
  • 新增 internal/logging/context_usage_test.go AST 静态检查 — 遍历 internal/ 下所有非测试 .go 文件,对接受 context.Context*http.Request 参数的函数,检测函数体内是否调用了 logger.Error/Info/Warn/Debug(无 context 变体)并报错。检查器通过 import alias 感知 contextnet/http 的重命名,通过 receiver/selector/ident 模式识别 logger 表达式。
  • 新增 internal/api/server_test.goTestNewServerUsesDefaultLoggerForHTTPAccess 验证 NewServerLogger 为 nil 时回落到 slog.Default(),并对 /healthz 请求断言恰好产生 2 条 component=http 的 access log(request + response)。
  • worker 包级函数重构为方法batches.WorkerRunOnce/runExpirySweepOnce/processJob/startHeartbeat/uploadResults/writeResultsJSONL)、cleanup.WorkerRunOnce)、filestore.CleanupWorkerRunCleanupOnce/RunFilesystemCleanupOnce/RunTTLSweepOnce)均从包级函数转为持有 database/store/cfg/upstream/logger 的方法;NewWorker 签名相应调整(如 batches.NewWorker 改为接收 config.BatchConfig + UpstreamClient),所有调用方(main.go、测试)已同步更新。
  • filestore handler 方法化并传入 contextwriteFilestoreResultlogFilestoreRequestErrorextendFilestoreDeadlines 转为 Handler 方法并新增 ctx context.Context 参数,所有调用点传入 r.Context()readFile 的 stream 错误日志也改用 ErrorContext

Pullfrog  | View workflow run | Using anthropic/glm-5.2𝕏

# Conflicts:
#	internal/agents/handler.go
#	internal/api/server.go
#	internal/api/service_auth.go
#	internal/deployments/handler.go
#	internal/environments/runner.go
#	internal/files/handler.go
#	internal/sessions/event_payload.go
#	internal/sessions/service.go
#	internal/workbench/console_platform_workbench.go
#	internal/workbench/console_platform_workbench_test.go
#	main.go
#	tests/environments_e2b_integration_test.go
#	tests/environments_full_e2b_bridge_integration_test.go
#	tests/environments_runner_cloud_test.go
#	tests/sdk_go_quickstart_demo_test.go
#	tests/sessions_api_test.go
# Conflicts:
#	internal/agents/handler.go
#	internal/api/server.go
#	internal/deployments/handler.go
#	internal/environments/runner_dependencies_test.go
#	internal/skillprewarm/worker.go
#	internal/skillprewarm/worker_test.go
#	internal/skills/handler.go
#	internal/skills/seed.go
#	main.go
#	tests/environments_e2b_integration_test.go
#	tests/environments_runner_cloud_test.go
@arthur-zhang
arthur-zhang merged commit da3b4ed into main Jul 27, 2026
7 checks passed
@arthur-zhang
arthur-zhang deleted the codex/a branch July 27, 2026 02:53

@duckpr duckpr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DuckPR reviewer: opencode
Model: anthropic/glm-5.2

✅ 未发现新问题。

Reviewed changes — 本次增量评审覆盖自上次 pullfrog 评审(4ab58d4)以来的 1 个新提交(5379fd8)。该提交将 origin/main(含已合入的 PR #160)合并入 codex/a,带来 internal/skillprewarm 包的整体删除以及构造函数签名的统一对齐。

  • 删除 internal/skillprewarm 子系统 — PR #160main 上移除了 skillprewarm 包(enqueuer.goworker.go 及对应测试),并用基于 internal/filestore/skill_archives.go 的存档存储替代。本次合并将此删除同步到本分支,main.go 不再调用 skillprewarm.StartWorker,所有对 skillprewarm import 的引用已清除。
  • 简化 handler 构造函数agents.NewHandlerskills.NewHandlerdeployments.NewHandler 不再有 NewHandlerWithSkillPrewarm 变体;prewarm 字段和 enqueueSkillPrewarm* 方法已删除;skills.NewRuntimeResolver(cfg, database, objectStore) 简化为 (database)。所有调用方(main.goserver.go、测试)已一致更新。
  • 删除孤立清理方法skills.deleteObjectOrEnqueueCleanup 随 prewarm 路径一并删除。

验证结果:go build ./... 通过,go vet ./... 通过,受影响包的单元测试全部通过,internal/logging 的 AST context-aware linter(TestContextAwareLoggerMethods)通过——确认合并引入的新代码(skill_archives.gopath_backend.gopersistent_backend.go 等)均遵守 slog *Context 规范,未引入 log.Printf 回退。上一轮评审中 skillprewarm/worker_test.go nil logger 噪音的反馈已随包删除自动解决。

Pullfrog  | View workflow run | Using anthropic/glm-5.2𝕏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant